feat(desktop): fetch custom relay models before connect by localhost-copilot · Pull Request #3299 · apache/maka

localhost-copilot · 2026-08-20T08:34:14Z

Summary

Add model discovery for unsaved custom relay providers.

Users can fetch models using the configured endpoint, API key, and request headers, then
select a default model from the returned catalog. If discovery fails or /models is
unavailable, manual model entry remains available and required.

The preview uses transient Runtime Host verification and does not persist credentials or
connection data. The Runtime Host compatibility epoch is bumped so older hosts reject the
new preview inputs safely.

Verification

npm --workspace @maka/runtime-host test — 1022 passed
npm --workspace @maka/desktop test — 972 passed
npm --workspace @maka/desktop run typecheck
npm run lint
npm run format:check
npx knip --workspace apps/desktop
git diff --check

UI evidence:

Astro-Han

Reviewed at d20fd8c8. The feature is well-shaped and the two things most likely to go wrong in a change like this are both handled correctly — I checked them specifically rather than assuming:

A user-supplied endpoint never receives a stored credential. #discoverOnboarding sets candidate = undefined whenever input.baseUrl is present, so exportCredentialMaterial is never consulted and secret collapses to the supplied key alone. Without that guard, { baseUrl: <attacker>, apiKey: null } would have shipped the saved provider key to an arbitrary host. It reads as deliberate, and it is the right guard.
Preview-only fields cannot reach persistence. Changing ConnectionOnboardingSaveInput from extends ConnectionOnboardingVerifyInput to a standalone interface means baseUrl and requestHeaders are structurally absent on the save path, so a transient endpoint cannot be smuggled into a stored connection. That is the correct way to express "preview only", better than a runtime check.

The blocking problem is not in this PR's own logic — it is a collision with a PR that is open right now. #3236 also bumps RUNTIME_HOST_COMPATIBILITY_EPOCH from 27 to 28, for an unrelated reason (staged access.credential.prepare/finalize). Both branches write the literal 28, so a textual merge is clean and silent, and the second one to land ships an epoch that no longer distinguishes two independent, mutually-incompatible protocol changes. Details inline; this needs coordinating before either merges.

The remaining architectural question is one of contract rather than correctness. validateConnectionBaseUrl allows any http:/https: URL with no restriction on the host, so this operation lets a Client make the Runtime Host issue an outbound request to an arbitrary address with arbitrary headers. That capability is not new — a user could already create a connection with any baseUrl, call connection.models.fetch, and delete it. What changes is that it now requires no catalog mutation and leaves no trace, and it arrives at the same time as #3236 makes remote Runtime Hosts a first-class deployment. Whether a remote Host should accept arbitrary outbound targets from its Client is a decision worth making explicitly rather than inheriting.

Reviewed with Claude Opus as an analysis assistant. Every claim here was verified by reading source at this head — including assertExactKeys, validateConnectionBaseUrl, the epoch comparison in client/connection.ts, and #3236's own diff. Nothing was executed; the epoch collision is a reading of both branches, not an observed merge.

Astro-Han · 2026-08-20T08:40:53Z

-export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 27 as const;
-// 27: Runtime Policy carries the Host-owned shell preference used by tool,
-// PTY, and prompt composition. Older peers cannot safely preserve that field.
+export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 28 as const;


[P1] Coordinate this epoch bump with #3236, which raises the same constant to the same value for a different reason. #3236 changes 27 → 28 for staged access.credential.prepare/finalize; this PR changes 27 → 28 for the transient endpoint and request-header fields. Both branches write the literal 28, so git merges them without a conflict and the second to land silently ships one epoch covering two independent incompatibilities. The concrete failure: a Client built from this branch and a Host built from #3236 both advertise 28 and are admitted by compatibilityEpoch !== RUNTIME_HOST_COMPATIBILITY_EPOCH, then the Host's requireExactRecord rejects baseUrl as an unknown field and aborts the transport — which is precisely the outcome the epoch exists to replace with a structured incompatible frame. Confirmed by reading both branches at their current heads; not reproduced by merging. Whichever PR lands second must take 29 and append its own comment line rather than accepting the textual merge. A test that pins the epoch to a literal would turn this silent collision into a failing check; there is currently none.

Astro-Han · 2026-08-20T08:40:53Z

    const slug = deriveConnectionSlug(input.providerType);
    const catalog = await this.#stores.connectionCatalog.getSnapshot();
-    const candidate = catalog.connections.find((connection) => connection.slug === slug);
+    const candidate = input.baseUrl


[P2] Decide explicitly whether a Client may direct the Host's outbound requests at an arbitrary address. validateConnectionBaseUrl constrains only scheme and length, so any http:/https: URL is accepted here — including link-local and private-range addresses such as a cloud metadata endpoint — and createRequestCustomizationFetch attaches caller-supplied headers to that request. This is a contract decision, not a defect: the capability already exists via create-connection plus connection.models.fetch, and the credential guard immediately below this line correctly prevents a stored key from reaching a supplied endpoint. What changes is that the request now requires no catalog mutation and leaves no persisted trace, arriving as #3236 makes remote Runtime Hosts first-class — so the actor and the Host are increasingly on different machines and networks. Confirmed by reading code at this head; not exercised against a live Host. Either state in docs/runtime-host-remote-access.md that a Client may originate arbitrary outbound HTTP from the Host, or constrain preview targets. Regression test: whichever rule you choose, assert it here — a preview against a link-local address should have a defined, tested outcome.

Astro-Han · 2026-08-20T08:40:53Z

  const input = requireExactRecord(value, 'connection onboarding verification input', [
    'providerType',
    'apiKey',
+    ...('baseUrl' in fields ? ['baseUrl'] : []),


[P3] Pass the full key list to requireExactRecord instead of deriving it from the value being validated. assertExactKeys only rejects keys that are not in the allowlist — it never requires a listed key to be present — so ...('baseUrl' in fields ? ['baseUrl'] : []) admits exactly the same inputs as listing 'baseUrl' unconditionally. The conditional and the extra requireRecord call above it are therefore dead machinery, and worse, they read as though the allowlist adapts to the payload, which is the one thing an exact-record check must never do. The next reader auditing this decoder for injectable fields has to work out that it is a no-op before they can trust it. Confirmed by reading codec.ts at this head. List all four keys directly and drop the fields binding.

Astro-Han

Re-reviewed at f96af70e. The P1 is resolved. The epoch is now 29, and the comment block reserves 28 for #3236's staged access-credential pairing — which is better than just moving your own number, because it makes the next person's collision impossible to create silently. Nothing else moved: the delta is protocol/index.ts and protocol.test.ts only.

My other two findings still stand at this head and I am not re-filing them inline:

P2, the outbound-target contract: validateConnectionBaseUrl still constrains only scheme and length, so this operation lets a Client point the Host's /models fetch at any http:/https: address with caller-supplied headers. The credential guard remains correct — a supplied baseUrl still forces candidate = undefined, so no stored key can reach a supplied endpoint — this is about whether the Host should originate arbitrary outbound requests at all, which matters more as #3236 makes remote Hosts first-class.
P3, the requireExactRecord allowlist computed from the value's own keys, which is a no-op because assertExactKeys only rejects unknown keys.

Reviewed with Claude Opus as an analysis assistant; verified by diffing against the head I previously reviewed and re-reading both files at this one.

Astro-Han · 2026-08-20T11:00:39Z

Heads-up on a cross-PR collision — not a review comment on your change.

RUNTIME_HOST_COMPATIBILITY_EPOCH is 27 on main, and three open PRs based on main each take it to 28 with different wire changes: #3236 (access credential prepare/finalize), #3199 (goal.arm), #3133 (session trace cursor pages). #3299 sits at 29 on the assumption that exactly one 28 lands.

The trap is that this does not conflict. All three branches write the same text to that line, so git's three-way merge takes it silently; only the adjacent comment block conflicts, and keeping both comments is the natural resolution. Each PR's own assert epoch > 27 still passes. The result is two incompatible protocols sharing epoch 28 — and since client/connection.ts compares with strict inequality, a matching epoch admits the peer, and the unknown operation then fails decode and tears down the transport, bypassing the structured incompatibility path the epoch exists to provide.

Please re-check against main immediately before merge rather than at rebase time; whoever lands second needs to re-bump. Filed #3313 to stop doing this by hand.

(Posted with Claude Code (Opus 5) assistance; the epoch values were read from each branch head.)

Astro-Han · 2026-08-23T04:47:54Z

Independent review of 4a290b704c6fa26be09e07d06cc6a657352aae0a. One [P1] — which is an existing finding that has changed state rather than a new one — plus an argument for closing one of the open threads.

[P1] The epoch collision is no longer a risk; it has landed

This branch is based on a main where RUNTIME_HOST_COMPATIBILITY_EPOCH was 36 and bumps it to 37. main is now at 39 (packages/runtime-host/src/protocol/index.ts:94, verified directly). Both sides edit the same constant, so git no longer merges it silently — the PR is CONFLICTING / DIRTY right now.

Resolving the text by hand would not be enough: 37 already means something on main, so this branch needs a rebase onto current main and a bump to 40.

The earlier note about this was written when it was still a risk of collision. It is now a hard blocker. Reporting the state change rather than re-filing it as a new finding.

On the "can the renderer point the Host at an arbitrary address" thread — I believe this can be closed, with new evidence

I traced #discoverOnboarding in connection-effect-coordinator.ts on this head independently:

When a baseUrl override is supplied, candidate is forced to undefined (:197-199), so stored = null. The preview cannot borrow credentials from an existing connection with the same slug. A stored key cannot be aimed at an attacker-chosen URL — that path is closed in code, not by convention.
The transient apiKey the user types is sent only to the URL that same user typed. That is exactly the authority the existing "save the connection, then fetch models" path already has — and preview is narrower, since it persists nothing. Tests pin zero writes on both the catalog and vault sides, and pin that the response carries no secret.
Protocol level: file:// is rejected (with a test), the response frame is forbidden from carrying apiKey (with a test), and the epoch bump's semantic comment is accurate.

So there is no secret-exfiltration channel here. What remains is a product decision — whether a renderer may cause the Host to reach an arbitrary address at all — and the pre-existing create-then-fetch path already answers it the same way. My recommendation is to close that thread as decided rather than ask for code changes.

Still open from the earlier review, unchanged on this head

requireExactRecord's derived key list ('baseUrl' in fields ? ['baseUrl'] : []) is untouched. It is functionally correct — unknown keys are still rejected — but it derives what is permitted from the input itself, which is a roundabout way to state an allowlist. Not blocking; noting only that it has had no response.

Candidates raised and withdrawn

Preview is a new SSRF surface — withdrawn; authority does not exceed the existing create+fetch path, and stored secrets are isolated from it.
A 64 KB transient apiKey crossing IPC and the protocol — withdrawn; same path as the existing onboarding.verify, with protocol tests pinning the bound.
A preview failure showing only a banner could mislead — withdrawn; it falls back to manual entry with a warning, which is the right semantics.

Scope

+383/-24 across 20 files for "add a fetch-models button to a form" looks heavy until you see the split: most of it is protocol extension (epoch, codec, boundary tests) and two-sided plumbing. That is the legitimate cost of this seam, with no surplus abstraction.

CI

check-runs on this head: 0 — never run. No workflow run exists to approve on this SHA, so CI will only appear after the rebase. No red mark here has never meant green.

Reviewed at 2026-08-23 12:50 UTC. Blind line — provisional judgment sealed before the existing reviews were read. No overall verdict offered; note that this PR currently cannot be merged at all.

Astro-Han · 2026-08-23T07:07:55Z

Hi — this PR conflicts with current main and cannot be merged as-is.

I tested a rebase onto current main locally (in a throwaway worktree — your branch was not touched). It stops on these files:

packages/runtime-host/src/__tests__/protocol.test.ts
packages/runtime-host/src/protocol/index.ts

These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current main and resolve them yourself, then push. Once the branch is conflict-free and CI is green on the new head, I will pick it up for review.

git fetch upstream && git rebase upstream/main
# resolve, then
git push --force-with-lease

Thanks for the contribution — happy to help if any conflict is unclear.

AI-assisted maintenance note, not a review. It does not count as the required human review under CONTRIBUTING.md §Review.

jackwener

Independent review of aae5cadea973ea5843ed0de7d2292545c9f2038b.

GO. No P0–P2. Hosted test is terminal success on this head. PR is CONFLICTING — I am not merging. After rebase, RUNTIME_HOST_COMPATIBILITY_EPOCH will need a fresh bump: this SHA goes 42→43, current main is already 44.

What this solves

Unsaved custom relays had no way to discover /models before create. Preview reuses connection.onboarding.verify with optional baseUrl / requestHeaders, does not persist catalog or vault state, and the epoch bump is the right handshake fence for the new optional fields.

Stall / timeout / degrade (the question for this PR)

Preview is opt-in, not on the save path. Save still goes create then existing fetchModels.

Timeout, not hang: runConnectionModelDiscoveryEffect uses MODEL_FETCH_TIMEOUT_MS = 10_000. A slow or dead /models fails; it does not wait forever.
Failure is not silent: warning banner + modelsFetchFallback; the default-model field stays a manual text input. Save remains available.
Connect does not wait on preview. The only coupling is the shared submitGuard (inline P3): while preview is in flight, Save looks enabled but the click is dropped.

I independently agree the preview URL is not a new SSRF/exfil surface versus create-then-fetch: a supplied baseUrl skips the catalog candidate, so a stored key cannot be aimed at an attacker URL. Not re-filed.

No P0/P1/P2 on this head.

jackwener · 2026-08-23T20:19:05Z

+  }
+
  async function submit() {
    if (submitGuard.current !== null) return;


[P3] submit() and fetchModelOptions() share one submitGuard. While preview is in flight (up to the 10s Host fetch timeout), Save stays visually enabled (isDisabled={busy || isExperimental} — it does not include fetchingModels) but this early return drops the click with no feedback.

Not blocking: the user can wait, or close the form; a failed preview already degrades to manual model entry. If you touch this again, disable Save while fetchingModels (or give the two actions separate guards).

Astro-Han

#3299 `26f5279` — incremental re-review (bind exact head)

Gate: CLEAN, test green. 4 threads outstanding as noted.
Scope: Re-check 4 prior findings + delta since aae5cad only. Not full 20-file re-review.
Exclusions: Full-file audit out of scope unless incidental.

[P1] Epoch collision — resolved by time: PR now sets RUNTIME_HOST_COMPATIBILITY_EPOCH=45, main is 44. So not 28 vs 28 collision; value is strictly greater than base. Contention with other 45 claimants remains per-merge-order but not regression vs main. No block. Verified via contents?ref=26f5279 vs ref=main.

[P2] validateConnectionBaseUrl allows any http/https including link-local/private — still present (scheme+length only). Credential guard in createRequestCustomizationFetch still blocks leaking stored keys. Determination remains contract-level capability (building connection + models.fetch is intentional). No expansion of surface observed in delta (not reachable pre-onboarding). Not blocking per prior rationale.

[P3] requireExactRecord dead-chain + submitGuard sharing — unchanged in delta, P3 non-blocking.

Verdict: COMMENT — no P0-P2 in re-check scope. Prior P1 now moot due to main advancement.

中文

增量复核，旧 P1 随主线推进已失效。

Add transient model discovery for unsaved custom relay configurations, expose it through the Desktop bridge, and let users select a discovered model while preserving manual entry as fallback.

Astro-Han · 2026-08-24T13:20:27Z

A number collision to flag, not a review of the change itself.

This branch declares RUNTIME_HOST_COMPATIBILITY_EPOCH = 49, and so does #3651. main is now at 48, so 49 is the immediate next number and both branches are asking for it. Whichever merges first takes it; the other moves to 50 with its assertion changed to > 49.

This branch is approved and #3651 is not, so you are likely to get there first — but the number is only guaranteed at the moment of merge, so it is worth re-deriving then rather than trusting this message:

git fetch origin main
git show origin/main:packages/runtime-host/src/protocol/index.ts | grep COMPATIBILITY_EPOCH
gh pr list --repo apache/maka --limit 200 --json number --jq '.[].number' | while read n; do
  gh api repos/apache/maka/pulls/$n/files --paginate \
    --jq '.[]|select(.filename=="packages/runtime-host/src/protocol/index.ts")|.patch' 2>/dev/null \
  | grep -oP '^\+export const RUNTIME_HOST_COMPATIBILITY_EPOCH = \K\d+' | sed "s/^/#$n /"
done

I have told #3651 the same thing, so neither of you is being asked to renumber pre-emptively.

简体中文

提醒一个编号冲突，不是对改动本身的评审。

这个分支声明了 RUNTIME_HOST_COMPATIBILITY_EPOCH = 49，而 #3651 也声明了同一个值。main 目前是 48，所以 49 是紧接着的下一个号，两个分支都在要它。先合并的那个拿到它，另一个改成 50，并把断言改为 > 49。

这个分支已获批准而 #3651 还没有，所以大概率是你先到——但这个号只有在合并那一刻才有保证，因此建议届时重新推导一次，而不是相信这条消息：

git fetch origin main
git show origin/main:packages/runtime-host/src/protocol/index.ts | grep COMPATIBILITY_EPOCH
gh pr list --repo apache/maka --limit 200 --json number --jq '.[].number' | while read n; do
  gh api repos/apache/maka/pulls/$n/files --paginate \
    --jq '.[]|select(.filename=="packages/runtime-host/src/protocol/index.ts")|.patch' 2>/dev/null \
  | grep -oP '^\+export const RUNTIME_HOST_COMPATIBILITY_EPOCH = \K\d+' | sed "s/^/#$n /"
done

我已经对 #3651 说了同样的话，所以你们两边都不需要现在就抢着改号。

Astro-Han reviewed Aug 20, 2026

View reviewed changes

Astro-Han mentioned this pull request Aug 20, 2026

feat(desktop): add managed remote Runtime Host onboarding #3236

Merged

6 tasks

Astro-Han reviewed Aug 20, 2026

View reviewed changes

This was referenced Aug 20, 2026

Derive the Runtime Host compatibility epoch instead of hand-editing it #3313

Closed

feat(goal): let the user arm a Goal from the composer #3199

Merged

This was referenced Aug 20, 2026

fix(runtime-host): page Session traces at the source #3133

Merged

test(runtime-host): guard v0.1.11 handshake compatibility #3322

Merged

localhost-copilot force-pushed the Connect-Custom-relay-fetch-models branch 3 times, most recently from d9a595e to 4a290b7 Compare August 22, 2026 15:44

localhost-copilot force-pushed the Connect-Custom-relay-fetch-models branch 2 times, most recently from b96d916 to aae5cad Compare August 23, 2026 14:28

jackwener approved these changes Aug 23, 2026

View reviewed changes

localhost-copilot force-pushed the Connect-Custom-relay-fetch-models branch from aae5cad to 26f5279 Compare August 24, 2026 05:00

Astro-Han reviewed Aug 24, 2026

View reviewed changes

feat(desktop): fetch custom relay models before connect

89642f0

Add transient model discovery for unsaved custom relay configurations, expose it through the Desktop bridge, and let users select a discovered model while preserving manual entry as fallback.

localhost-copilot force-pushed the Connect-Custom-relay-fetch-models branch from 26f5279 to 89642f0 Compare August 24, 2026 13:10

Astro-Han mentioned this pull request Aug 24, 2026

feat(desktop): show context compaction in transcript #3651

Open

6 tasks

hgaol mentioned this pull request Aug 24, 2026

feat(runetime-host): sign in to GitHub Copilot with a device grant #3390

Open

6 tasks

Astro-Han mentioned this pull request Aug 24, 2026

fix(desktop): show armed goals waiting for their first Turn #3368

Open

6 tasks

Navigation Menu

Uh oh!

feat(desktop): fetch custom relay models before connect - #3299

feat(desktop): fetch custom relay models before connect#3299
localhost-copilot wants to merge 1 commit into
apache:mainfrom
localhost-copilot:Connect-Custom-relay-fetch-models

localhost-copilot commented Aug 20, 2026

Uh oh!

Astro-Han left a comment

Uh oh!

Astro-Han Aug 20, 2026

Uh oh!

Astro-Han Aug 20, 2026

Uh oh!

Astro-Han Aug 20, 2026

Uh oh!

Astro-Han left a comment

Uh oh!

Astro-Han commented Aug 20, 2026

Uh oh!

Astro-Han commented Aug 23, 2026

Uh oh!

Astro-Han commented Aug 23, 2026

Uh oh!

jackwener left a comment

Uh oh!

jackwener Aug 23, 2026

Uh oh!

Astro-Han left a comment

Uh oh!

Astro-Han commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

localhost-copilot commented Aug 20, 2026

Summary

Verification

Uh oh!

Astro-Han left a comment

Choose a reason for hiding this comment

Uh oh!

Astro-Han Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

Astro-Han Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

Astro-Han Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

Astro-Han left a comment

Choose a reason for hiding this comment

Uh oh!

Astro-Han commented Aug 20, 2026

Uh oh!

Astro-Han commented Aug 23, 2026

[P1] The epoch collision is no longer a risk; it has landed

On the "can the renderer point the Host at an arbitrary address" thread — I believe this can be closed, with new evidence

Still open from the earlier review, unchanged on this head

Candidates raised and withdrawn

Scope

CI

Uh oh!

Astro-Han commented Aug 23, 2026

Uh oh!

jackwener left a comment

Choose a reason for hiding this comment

What this solves

Stall / timeout / degrade (the question for this PR)

Uh oh!

jackwener Aug 23, 2026

Choose a reason for hiding this comment

Uh oh!

Astro-Han left a comment

Choose a reason for hiding this comment

#3299 26f5279 — incremental re-review (bind exact head)

Uh oh!

Astro-Han commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

#3299 `26f5279` — incremental re-review (bind exact head)